fix(core): retain reference scope during dereference by bvolpato · Pull Request #1268 · substrait-io/substrait-java

bvolpato · 2026-09-03T19:13:19Z

Dereferencing a scoped field rebuilds FieldReference without its outer-reference or lambda metadata. SQL such as i.id = o.s.v inside a correlated subquery consequently exports o.s.v as a local root reference; with matching schemas it instead reads i.s.v without a type error.

Copy the original reference metadata while extending its path and deriving the selected type. Struct, list, and map dereferences retain their outer steps, relation anchor, or lambda scope, including a zero-step reference to the current lambda.

Nested outer and lambda field paths remain unsupported when converting protobuf back to POJOs or POJOs back to Calcite. Reject these paths explicitly instead of dropping segments or selecting another field. Reading nested lambda parameter paths remains tracked in #1322.

Summary by CodeRabbit

Bug Fixes
- Corrected nested field dereferencing to preserve the reference’s existing scope and correlation context, improving handling of nested fields in correlated queries.
Tests
- Added coverage for dereferencing struct, list, and map fields across reference scopes, including correlated nested-field queries.

Dereferencing a scoped field rebuilds FieldReference without its outer-reference or lambda metadata. SQL such as i.id = o.s.v inside a correlated subquery consequently exports o.s.v as a local root reference; with matching schemas it instead reads i.s.v without a type error. Copy the original reference metadata while extending its path and deriving the selected type. Struct, list, and map dereferences retain their outer steps, relation anchor, or lambda scope, including a zero-step reference to the current lambda. This fixes reference construction and SQL export. It does not add support for nested scoped paths in reverse converters that currently cannot handle them.

coderabbitai · 2026-09-29T05:21:24Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Caution

Review failed

An error occurred during the review process. Please try again later.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 166d0662-2d4d-44f6-9f4b-44e0db9ee854

📥 Commits

Reviewing files that changed from the base of the PR and between 4d21734 and 527eaf7.

📒 Files selected for processing (3)

core/src/main/java/io/substrait/expression/FieldReference.java
core/src/test/java/io/substrait/expression/FieldReferenceDereferenceTest.java
isthmus/src/test/java/io/substrait/isthmus/CorrelatedNestedFieldTest.java

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

FieldReference.dereference now copies the existing reference before replacing its segments. New tests check dereference behavior across reference scopes and verify correlated nested-field references in a converted plan.

Changes

Field reference dereferencing

Layer / File(s)	Summary
Dereference behavior and validation `core/src/main/java/io/substrait/expression/FieldReference.java`, `core/src/test/java/io/substrait/expression/FieldReferenceDereferenceTest.java`, `isthmus/src/test/java/io/substrait/isthmus/CorrelatedNestedFieldTest.java`	`dereference` copies the current reference, sets the new type, and prepends the new segment. Tests check segment ordering, scope preservation, serialized scope fields, and correlated nested-field references in a converted plan.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: nielspardon

Merge Risk: 🟡 Moderate · up to 527ea

Forward conversion preserves reference scope, but reverse conversion can silently change nested correlated references or reject nested lambda references. Handle or explicitly reject unsupported paths before merging to prevent incorrect round-trip results.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 527ea

The change repairs reference binding without demonstrating new access privileges or an authorization bypass. Some reverse conversions can still reinterpret unsupported nested references without rejecting them. Production exposure and end-to-end behavior remain unconfirmed.

Retained concerns

Low · reliability · inferred: Scope-preserving dereference outputs can reach existing reverse-conversion branches that consume only one segment instead of rejecting unsupported nested paths. This creates a producer/consumer failure-containment gap: reverse conversion may return a different reference. The reader defects predate the PR, but this dereference path now generates the scoped forms that select them. No authorization or data-exposure consequence was established.

Security review details

Security Blast Radius

inferred — The demonstrated propagation scope is library-level reference construction, serialization, and correlated-expression conversion. The evidence does not establish an independently attackable production endpoint, tenant boundary, privileged sink, or deployment-wide exposure.

Trust Boundaries and Controls

observed — Correlated-field conversion rejects expressions without a relation visitor or a binding relation anchor. Scope preservation does not remove these checks. They establish reference binding, not tenant authorization.

Resilience and Maintainability Implications

observed — Rex conversion's type-observation hook is not an unconditional rejection mechanism: it returns immediately for the no-op observer. It therefore does not universally contain the scoped-path loss identified in the conversion branches.

Hardening Proposals

proposed — Reverse-conversion boundaries could reject unsupported nested scoped paths explicitly until complete path preservation is implemented, preventing silent reference substitution.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise, uses a valid Conventional Commit format, and clearly describes the main change: preserving reference scope during dereference.
Description check	✅ Passed	The description explains the defect, the behavior being fixed, the affected reference scopes, and the explicit limitation. It provides the required rationale and is relevant to the changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

nielspardon

Thanks, keeping the scope is right: the spec allows a nested direct_reference under outer_reference and lambda_parameter_reference. The reverse converters don't reject these paths, though. They now read the wrong field without any error, so please add guards (or real handling) in this PR.

nielspardon · 2026-09-29T11:50:16Z


  private FieldReference dereference(Type newType, ReferenceSegment nextSegment) {
    return ImmutableFieldReference.builder()
+        .from(this)


Reject nested scoped paths in these readers, or handle them. Before this change they threw on these references; now they return a wrong result. These lines are outside the diff, so I couldn't attach suggestions:

ExpressionRexConverter.java:834 (outer) and :863 (lambda) take segments().get(0), which is the innermost step. The new isthmus query converts back to Calcite as i.id = $cor0.ID, and a lambda x.f1 becomes (p0, p1) -> p1. Throwing when segments().size() > 1 covers both.

ProtoExpressionConverter.java:94 reads only the top struct_field and drops its child, so the exported plan reads back as i.id = o.s (INTEGER vs ROW). The lambda case at :119 already throws on hasChild(); do the same here.

An assertProtoPlanRoundrip on the new isthmus query would have caught the second one.

Thanks for tracing the consumer paths. The local patch adds guards in both readers and regressions for nested outer and lambda references, including matching-type fields that previously selected the wrong field without an error. Unsupported paths now fail explicitly. The full build passes, and the update is not pushed yet.

nielspardon · 2026-09-29T11:50:16Z

+      case OUTER_ANCHOR:
+        builder.outerReferenceRelReference(7);
+        break;
+      case LAMBDA_CURRENT:


A dereferenced lambda parameter now writes a nested lambda_parameter_reference path, which ProtoExpressionConverter:119 still rejects, so these plans no longer survive POJO → proto → POJO. That reader side is item 3 of #1322; either handle it here or note it as a known gap in the PR description.

Agreed. I prepared description text that explicitly keeps nested lambda parameter reading unsupported and points to #1322. This change preserves those paths during construction and export, while rejecting them where readers cannot handle them. The description update is pending publication.

nielspardon · 2026-09-29T11:50:16Z

+    FieldReference reference = reference(scope, R.struct(R.BOOLEAN, N.I64));
+
+    assertDereference(
+        reference, reference.dereferenceStruct(1), N.I64, FieldReference.StructField.of(1));


Dereference a different field than the base's StructField(1), so the expected list isn't symmetric and a segment-order regression fails this case too.

Suggested change

reference, reference.dereferenceStruct(1), N.I64, FieldReference.StructField.of(1));

reference, reference.dereferenceStruct(0), R.BOOLEAN, FieldReference.StructField.of(0));

Thanks, I changed the local struct case to child index 0 and R.BOOLEAN, so both the offset and type distinguish the new segment from the base reference. This is included in the pending update.

nielspardon · 2026-09-29T11:50:16Z

+    assertEquals(expectedType, dereferenced.getType());
+    assertEquals(List.of(nextSegment, original.segments().get(0)), dereferenced.segments());
+    assertEquals(original.inputExpression(), dereferenced.inputExpression());
+    assertEquals(original.outerReferenceStepsOut(), dereferenced.outerReferenceStepsOut());
+    assertEquals(original.outerReferenceRelReference(), dereferenced.outerReferenceRelReference());
+    assertEquals(
+        original.lambdaParameterReferenceStepsOut(),
+        dereferenced.lambdaParameterReferenceStepsOut());
+
+    io.substrait.proto.Expression.FieldReference originalProto =
+        expressionProtoConverter.toProto(original).getSelection();
+    io.substrait.proto.Expression.FieldReference dereferencedProto =
+        expressionProtoConverter.toProto(dereferenced).getSelection();
+    assertEquals(originalProto.getRootTypeCase(), dereferencedProto.getRootTypeCase());
+    assertEquals(originalProto.getOuterReference(), dereferencedProto.getOuterReference());
+    assertEquals(
+        originalProto.getLambdaParameterReference(),
+        dereferencedProto.getLambdaParameterReference());


Compare the whole object instead of listing attributes by hand, so a newly added attribute that dereference drops is caught. Also remove the then-unused java.util.List import.

Suggested change

assertEquals(expectedType, dereferenced.getType());

assertEquals(List.of(nextSegment, original.segments().get(0)), dereferenced.segments());

assertEquals(original.inputExpression(), dereferenced.inputExpression());

assertEquals(original.outerReferenceStepsOut(), dereferenced.outerReferenceStepsOut());

assertEquals(original.outerReferenceRelReference(), dereferenced.outerReferenceRelReference());

assertEquals(

original.lambdaParameterReferenceStepsOut(),

dereferenced.lambdaParameterReferenceStepsOut());

io.substrait.proto.Expression.FieldReference originalProto =

expressionProtoConverter.toProto(original).getSelection();

io.substrait.proto.Expression.FieldReference dereferencedProto =

expressionProtoConverter.toProto(dereferenced).getSelection();

assertEquals(originalProto.getRootTypeCase(), dereferencedProto.getRootTypeCase());

assertEquals(originalProto.getOuterReference(), dereferencedProto.getOuterReference());

assertEquals(

originalProto.getLambdaParameterReference(),

dereferencedProto.getLambdaParameterReference());

assertEquals(

ImmutableFieldReference.copyOf(original)

.withType(expectedType)

.withSegments(nextSegment, original.segments().get(0)),

dereferenced);

Thanks, I switched the local test to whole-reference equality with copyOf, withType, and withSegments, and removed the unused List import. The branch update is pending.

nielspardon · 2026-09-29T11:50:17Z

+  }
+
+  private FieldReference reference(ReferenceScope scope, Type type) {
+    ImmutableFieldReference.Builder builder =


Nit: build these with the existing factories (newRootStructReference, newStructReference, newRootStructOuterReference, newRootStructOuterReferenceByRelReference, newLambdaParameterReference) in a switch expression, which also drops the unreachable default.

Agreed, I updated the local fixture to use the reference factories through a switch expression, including the lambda factory in this package. This is included in the pending update.

nielspardon · 2026-09-29T11:50:17Z

+import org.apache.calcite.sql.parser.SqlParseException;
+import org.junit.jupiter.api.Test;
+
+class CorrelatedNestedFieldTest {


Nit: move this into SubqueryPlanTest as a @Test using toProto(toSubstraitPlan(sql, catalog)), since it repeats that class's correlated-EXISTS navigation step for step.

Thanks, I moved the local regression into SubqueryPlanTest and reused the shared conversion helpers. The reverse-reader assertions now sit alongside the export regression. The full build passes; these changes are not pushed yet.

nielspardon · 2026-09-30T16:19:37Z

@coderabbitai resume

coderabbitai · 2026-09-30T16:19:47Z

✅ Action performed

Reviews resumed and review finished.

Reject nested outer and lambda paths before reverse conversion can select the wrong field. Cover the reader failures and simplify scoped dereference regressions.

bvolpato force-pushed the bvolpato/fix-correlated-dereference branch from 07dce85 to b335a63 Compare September 4, 2026 16:30

vinaykpud mentioned this pull request Sep 25, 2026

isthmus: support struct field access from RexLambdaRef #1322

Open

bvolpato force-pushed the bvolpato/fix-correlated-dereference branch from b335a63 to 527eaf7 Compare September 29, 2026 05:21

bvolpato marked this pull request as ready for review September 29, 2026 05:30

nielspardon reviewed Sep 29, 2026

View reviewed changes

fix: reject unsupported nested scoped reference reads

2da6d33

Reject nested outer and lambda paths before reverse conversion can select the wrong field. Cover the reader failures and simplify scoped dereference regressions.

	reference, reference.dereferenceStruct(1), N.I64, FieldReference.StructField.of(1));
	reference, reference.dereferenceStruct(0), R.BOOLEAN, FieldReference.StructField.of(0));

Conversation

bvolpato commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

nielspardon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielspardon commented Sep 30, 2026

Uh oh!

coderabbitai Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bvolpato commented Sep 3, 2026 •

edited

Loading

coderabbitai Bot commented Sep 29, 2026 •

edited

Loading

coderabbitai Bot commented Sep 30, 2026 •

edited

Loading